[java] JSpecify annotations for `Credential` and `MBean` #16481

mk868 · 2025-10-21T18:38:11Z

User description

🔗 Related Issues

Related #14291

💥 What does this PR do?

JSpecify annotations added to the:

org.openqa.selenium.grid.jmx.MBean
org.openqa.selenium.virtualauthenticator.Credential

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

New feature (non-breaking change which adds functionality and tests!)

PR Type

Enhancement

Description

Add JSpecify null-safety annotations to MBean class
Add JSpecify null-safety annotations to Credential class
Improve null-safety handling in attribute and operation invocation methods
Add defensive null checks for map lookups in getter/setter/invoke operations

Diagram Walkthrough

flowchart LR
  A["MBean.java<br/>Credential.java"] -->|Add @NullMarked| B["Class-level<br/>null-safety"]
  A -->|Add @Nullable| C["Method returns<br/>and parameters"]
  A -->|Add null checks| D["getAttribute/setAttribute<br/>invoke methods"]
  C --> E["Improved type<br/>safety"]
  D --> E

File Walkthrough

Relevant files

Enhancement

MBean.java `Add JSpecify null-safety annotations and null checks` java/src/org/openqa/selenium/grid/jmx/MBean.java Add `@NullMarked` class-level annotation for null-safety Add `@Nullable` annotations to method return types and parameters (getter, setter, and various finder methods) Add defensive null checks in `getAttribute()`, `setAttribute()`, and `invoke()` methods to handle null map lookups Improve null-safety in `AttributeInfo` and `OperationInfo` handling	+31/-15
Credential.java `Add JSpecify null-safety annotations to Credential` java/src/org/openqa/selenium/virtualauthenticator/Credential.java Add `@Nullable` annotation to `rpId` field and `getRpId()` method return type Add `@Nullable` annotation to `toMap()` method return type for map values Update constructor parameter to reflect nullable `rpId`	+4/-4

qodo-merge-pro · 2025-10-21T18:38:43Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance

⚪

Incomplete error handling

Description: The change makes DynamicMBean#getAttribute return null when the attribute is missing or
the getter is null, which can silently mask errors and enable information disclosure
patterns if callers assume non-null values; consider throwing AttributeNotFoundException
per JMX conventions instead of returning null.
MBean.java [220-241]

Referred Code

@Override
public @Nullable Object getAttribute(String attribute) {
  try {
    AttributeInfo attributeInfo = attributeMap.get(attribute);
    if (attributeInfo == null || attributeInfo.getter == null) {
      return null;
    }
    Object res = attributeInfo.getter.invoke(bean);
    if (res instanceof Map<?, ?>) {
      return ((Map<?, ?>) res)
          .entrySet().stream()
              .collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().toString()));
    } else if (res instanceof Number) {
      return res;
    } else {
      return res.toString();
    }
  } catch (IllegalAccessException | InvocationTargetException e) {
    LOG.severe("Error during execution: " + e.getMessage());
    return null;
  }


 ... (clipped 1 lines)

Incomplete error handling

Description: The invoke method now returns null when an operation is not found, potentially hiding
management misconfigurations and enabling ambiguous behavior; per JMX, it should signal
errors (e.g., MBeanException or ReflectionException) rather than returning null.
MBean.java [276-287]

Referred Code

@Override
public @Nullable Object invoke(String actionName, Object[] params, String[] signature) {
  try {
    OperationInfo operationInfo = operationMap.get(actionName);
    if (operationInfo == null) {
      return null;
    }
    return operationInfo.method.invoke(bean, params);
  } catch (IllegalAccessException | InvocationTargetException e) {
    LOG.severe("Error during execution: " + e.getMessage());
    return null;
  }

Ticket Compliance

🟢

🎫 #14291

🟢	Add JSpecify Nullness annotations across Selenium Java code to declare nullable parameters and return values.
	Use @nullable on APIs that can return or accept null to improve IDE/static analyzer detection.
	Use @NullMarked (or equivalent) to enforce non-null by default within scopes where possible.
	Improve null-safety handling to avoid NPEs at runtime where applicable.

Codebase Duplication Compliance

⚪

Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance

⚪

No custom compliance provided

Follow the guide to enable custom compliance check.

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

qodo-merge-pro · 2025-10-21T18:40:15Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Missing return statements in method Add missing `return` statements in the `findSetter` method for branches handling method names starting with `get` and `is` to ensure the found setter method is correctly returned. java/src/org/openqa/selenium/grid/jmx/MBean.java [158-175] private @Nullable Method findSetter(Method annotatedMethod) { ManagedAttribute ma = annotatedMethod.getAnnotation(ManagedAttribute.class); if (!"".equals(ma.setter())) { return findMethod(annotatedMethod.getDeclaringClass(), ma.setter()); } else { String name = annotatedMethod.getName(); if (name.startsWith("set")) { return annotatedMethod; } if (name.startsWith("get")) { - findMethod(annotatedMethod.getDeclaringClass(), "s" + name.substring(1)); + return findMethod(annotatedMethod.getDeclaringClass(), "s" + name.substring(1)); } if (name.startsWith("is")) { - findMethod(annotatedMethod.getDeclaringClass(), "set" + name.substring(2)); + return findMethod(annotatedMethod.getDeclaringClass(), "set" + name.substring(2)); } } return null; } Apply / Chat Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a significant bug in the `findSetter` method where `return` statements are missing, causing the logic for `get` and `is` prefixed methods to fail and always return `null`.	High
Learned best practice	Fail on invalid attribute sets Validate and throw precise exceptions when an attribute is unknown or not writable, and include the attribute name to aid debugging. java/src/org/openqa/selenium/grid/jmx/MBean.java [244-254] public void setAttribute(Attribute attribute) { + String name = attribute.getName(); + AttributeInfo attributeInfo = attributeMap.get(name); + if (attributeInfo == null) { + throw new IllegalArgumentException("Unknown attribute: " + name); + } + if (attributeInfo.setter == null) { + throw new IllegalStateException("Attribute not writable: " + name); + } try { - AttributeInfo attributeInfo = attributeMap.get(attribute.getName()); - if (attributeInfo == null \|\| attributeInfo.setter == null) { - return; - } attributeInfo.setter.invoke(bean, attribute.getValue()); } catch (IllegalAccessException \| InvocationTargetException e) { LOG.severe("Error during execution: " + e.getMessage()); + throw new IllegalStateException("Failed to set attribute: " + name, e); } } Apply / Chat Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Validate inputs and states early; avoid silent no-ops and null returns in command methods to prevent logic errors and hidden failures.	Low
Learned best practice	Throw explicit errors on invalid attributes Fail fast with precise exceptions when an attribute is missing or not readable instead of returning null; include the attribute name in the message. java/src/org/openqa/selenium/grid/jmx/MBean.java [221-241] -public @Nullable Object getAttribute(String attribute) { +public Object getAttribute(String attribute) { + AttributeInfo attributeInfo = attributeMap.get(attribute); + if (attributeInfo == null) { + throw new IllegalArgumentException("Unknown attribute: " + attribute); + } + if (attributeInfo.getter == null) { + throw new IllegalStateException("Attribute not readable: " + attribute); + } try { - AttributeInfo attributeInfo = attributeMap.get(attribute); - if (attributeInfo == null \|\| attributeInfo.getter == null) { - return null; + Object res = attributeInfo.getter.invoke(bean); + if (res instanceof Map<?, ?>) { + return ((Map<?, ?>) res) + .entrySet().stream() + .collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().toString())); + } else if (res instanceof Number) { + return res; + } else { + return res.toString(); } - Object res = attributeInfo.getter.invoke(bean); - ... } catch (IllegalAccessException \| InvocationTargetException e) { LOG.severe("Error during execution: " + e.getMessage()); - return null; + throw new IllegalStateException("Failed to get attribute: " + attribute, e); } } `[To ensure code accuracy, apply this suggestion manually]` Suggestion importance[1-10]: 5 __ Why: Relevant best practice - Validate inputs and states early to avoid NPEs and logic errors; avoid returning null where JMX contract expects informative errors.	Low
More

selenium-ci added B-grid Everything grid and server related C-java Java Bindings labels Oct 21, 2025

[java] JSpecify annotations for Credential and MBean

756b7a9

mk868 force-pushed the jspecify-Credential-MBean branch from 1879c49 to 756b7a9 Compare October 21, 2025 18:38

qodo-merge-pro bot added the Review effort 2/5 label Oct 21, 2025

diemol approved these changes Oct 22, 2025

View reviewed changes

diemol merged commit c1160a5 into SeleniumHQ:trunk Oct 22, 2025
39 checks passed

mk868 deleted the jspecify-Credential-MBean branch October 22, 2025 08:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[java] JSpecify annotations for `Credential` and `MBean` #16481

[java] JSpecify annotations for `Credential` and `MBean` #16481

mk868 commented Oct 21, 2025 •

edited by qodo-merge-pro bot

Loading

Uh oh!

qodo-merge-pro bot commented Oct 21, 2025

Uh oh!

qodo-merge-pro bot commented Oct 21, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

[java] JSpecify annotations for Credential and MBean #16481

[java] JSpecify annotations for Credential and MBean #16481

Conversation

mk868 commented Oct 21, 2025 • edited by qodo-merge-pro bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

🔗 Related Issues

💥 What does this PR do?

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

PR Type

Description

Diagram Walkthrough

File Walkthrough

Uh oh!

qodo-merge-pro bot commented Oct 21, 2025

PR Compliance Guide 🔍

Uh oh!

qodo-merge-pro bot commented Oct 21, 2025

PR Code Suggestions ✨

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[java] JSpecify annotations for `Credential` and `MBean` #16481

[java] JSpecify annotations for `Credential` and `MBean` #16481

mk868 commented Oct 21, 2025 •

edited by qodo-merge-pro bot

Loading